Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix builds of lightning-rapid-gossip-sync and lightning-background-processor crates #3289

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Sep 4, 2024

Previously, we would only check the builds for the workspace as a whole. This however would mean that we would check/test crates with lightning's default features enabled, allowing failures-to-build under the crates own default features to slip through, if they didn't explicitly enablelightning/std, for example.

Here, we fix the regressions in the builds of the lightning-rapid-gossip-sync and lightning-background-processor crates, and extend the CI to run checks, tests, and doc generation on the workspace members individually, asserting that all of them build even when not built as part of the same workspace as lightning.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.91%. Comparing base (6662c5c) to head (545b037).
Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3289      +/-   ##
==========================================
+ Coverage   89.65%   89.91%   +0.26%     
==========================================
  Files         126      126              
  Lines      102546   104476    +1930     
  Branches   102546   104476    +1930     
==========================================
+ Hits        91935    93941    +2006     
+ Misses       7890     7841      -49     
+ Partials     2721     2694      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tnull tnull force-pushed the 2024-09-fix-rgs-bp-builds branch 2 times, most recently from feb6f75 to 21353b7 Compare September 4, 2024 09:34
lightning/Cargo.toml Outdated Show resolved Hide resolved
lightning-background-processor/Cargo.toml Outdated Show resolved Hide resolved
@tnull
Copy link
Contributor Author

tnull commented Sep 13, 2024

As discussed elsewhere I now added a commit dropping lightning's no-std feature and also added a fixup so that RGS no only depends on bitcoin-io/std (which also requires bitcoin_hashes/std).

@tnull tnull force-pushed the 2024-09-fix-rgs-bp-builds branch 3 times, most recently from 7ae3a50 to 0e8a736 Compare September 13, 2024 11:35
@TheBlueMatt
Copy link
Collaborator

CI is very sad :/

@tnull tnull force-pushed the 2024-09-fix-rgs-bp-builds branch 3 times, most recently from 1b2e18b to 5adb167 Compare September 16, 2024 08:37
@tnull
Copy link
Contributor Author

tnull commented Sep 16, 2024

Now added a commit fixing doc generation for lightning without std set. Fingers crossed this improves my standing with said CI.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM.

ci/ci-tests.sh Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
lightning/src/util/hash_tables.rs Show resolved Hide resolved
@tnull tnull force-pushed the 2024-09-fix-rgs-bp-builds branch 3 times, most recently from 8fefd84 to df6ea28 Compare September 17, 2024 13:13
@TheBlueMatt
Copy link
Collaborator

Feel free to squash imo.

@tnull
Copy link
Contributor Author

tnull commented Sep 17, 2024

Feel free to squash imo.

Squashed without further changes.

TheBlueMatt
TheBlueMatt previously approved these changes Sep 17, 2024
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual changes here are very straightforward. The patch isn't trivial in size cause of the doc cleanups, but those are all mechanical, so just gonna land this.

lightning-background-processor/Cargo.toml Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator

Oops, looks like fuzzing CI is still trying to pass a no-std feature.

We previously removed these, leading to failure to build under default
features including `std`.
Previously, we would only check the workspace as a whole. This however
would mean that we would check/test crates with `lightning`'s default
features enabled, allowing failures-to-build under the crates own
default features to slip through, if they didn't explicitly enable
`lightning/std`, for example.

Here, we extend the CI to check the workspace as a whole but then run
checks, tests, and doc generation on the workspace members individually,
asserting that all of them build even when not built as part of the same
workspace as `lightning`.
We drop the `lightning/no-std` feature and just take
`hashbrown`,`possiblyrandom` and `libm` as required dependencies.
@tnull
Copy link
Contributor Author

tnull commented Sep 18, 2024

Oops, looks like fuzzing CI is still trying to pass a no-std feature.

Ah, actually not fuzzing, but the check-compiles script.

Amended and force-pushed with the following changes:

diff --git a/ci/check-compiles.sh b/ci/check-compiles.sh
index 7ad9f4df1..a067861fb 100755
--- a/ci/check-compiles.sh
+++ b/ci/check-compiles.sh
@@ -7,4 +7,4 @@ cargo doc
 cargo doc --document-private-items
 cd fuzz && RUSTFLAGS="--cfg=fuzzing --cfg=secp256k1_fuzz --cfg=hashes_fuzz" cargo check --features=stdin_fuzz
-cd ../lightning && cargo check --no-default-features --features=no-std
+cd ../lightning && cargo check --no-default-features
 cd .. && RUSTC_BOOTSTRAP=1 RUSTFLAGS="--cfg=c_bindings" cargo check -Z avoid-dev-deps
diff --git a/lightning-background-processor/Cargo.toml b/lightning-background-processor/Cargo.toml
index d73499fd4..0afc18fdf 100644
--- a/lightning-background-processor/Cargo.toml
+++ b/lightning-background-processor/Cargo.toml
@@ -16,5 +16,5 @@ rustdoc-args = ["--cfg", "docsrs"]
 [features]
 futures = [ ]
-std = ["lightning/std", "lightning-rapid-gossip-sync/std"]
+std = ["lightning/std", "bitcoin-io/std", "bitcoin_hashes/std"]

 default = ["std"]
@@ -22,4 +22,6 @@ default = ["std"]
 [dependencies]
 bitcoin = { version = "0.32.2", default-features = false }
+bitcoin_hashes = { version = "0.14.0", default-features = false }
+bitcoin-io = { version = "0.1.2", default-features = false }
 lightning = { version = "0.0.124", path = "../lightning", default-features = false }
 lightning-rapid-gossip-sync = { version = "0.0.124", path = "../lightning-rapid-gossip-sync", default-features = false }

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, attempt number two.

The actual changes here are (still) very straightforward. The patch isn't trivial in size cause of the doc cleanups, but those are all mechanical, so just gonna land this.

@TheBlueMatt TheBlueMatt merged commit cdd1298 into lightningdevkit:main Sep 18, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants